Code Review

This is a well-motivated fix for the Aspire 13.2.0 breaking change. The root cause analysis is correct and clearly documented. A few things worth discussing:

Architecture: Opt-in vs Opt-out semantics (worth discussing)

TUnit.Aspire/AspireFixture.cs:536

The change flips the filtering logic from opt-out (skip known non-lifecycle resources) to opt-in (only wait for known compute resources):

// Before: skip resources that identify themselves as non-waitable
if (r is IResourceWithoutLifetime) { skip }

// After: only wait for resources that implement IComputeResource
if (r is not IComputeResource) { skip }

The concern is extensibility: any custom or future Aspire resource that is lifecycle-bound but doesn't implement IComputeResource would now be silently skipped rather than waited on. The old approach erred on the side of "wait unless explicitly marked as non-waitable"; the new approach errs on "skip unless explicitly a compute resource."

A more defensive approach that preserves the old semantics while fixing the bug:

if (r is not IComputeResource && r is not IResourceWithoutLifetime)
{
    // Unknown resource type — still try to wait for it
    waitable.Add(r.Name);
}
else if (r is not IComputeResource) // is IResourceWithoutLifetime
{
    skipped ??= [];
    skipped.Add(r.Name);
}

Or more simply: skip if IResourceWithoutLifetime OR if it's not IComputeResource and has some other signal.

At minimum, it's worth documenting in a comment that this is an intentional narrowing, and consider whether IComputeResource will remain stable across future Aspire versions — the same versioning problem that broke IResourceWithoutLifetime could happen again.

New test projects missing from TUnit.slnx

Neither TUnit.Aspire.Tests nor TUnit.Aspire.Tests.AppHost are added to TUnit.slnx. This means they won't appear in IDE solution explorers and may not be included in CI builds unless the workflow explicitly discovers them by path. Worth adding:

<Project Path="TUnit.Aspire.Tests/TUnit.Aspire.Tests.csproj" />
<Project Path="TUnit.Aspire.Tests.AppHost/TUnit.Aspire.Tests.AppHost.csproj" />

Test duplication between VerifyIComputeResource.cs and WaitForHealthyReproductionTests.cs

ParameterResource_IsNot_IComputeResource (in WaitForHealthyReproductionTests) and ContainerResource_Is_IComputeResource (in VerifyIComputeResource) test the same assertions. Consider merging into one file to reduce maintenance surface — the separate file doesn't add meaningful categorization here.

TestFixtureAllHealthy as a top-level class

// WaitForHealthyReproductionTests.cs, bottom of file
public class TestFixtureAllHealthy : AspireFixture<Projects.TUnit_Aspire_Tests_AppHost>

This is a test helper, not a test class. Having it top-level pollutes the test namespace and makes it discoverable by the test runner as a potential fixture. Better as a private or internal nested class inside WaitForHealthyReproductionTests.

Unused ct parameter in AspireFixture_AllHealthy_Succeeds_AfterFix

public async Task AspireFixture_AllHealthy_Succeeds_AfterFix(CancellationToken ct)
{
    var fixture = new TestFixtureAllHealthy();
    try
    {
        await fixture.InitializeAsync();  // ct not passed
    }

The ct parameter is accepted (TUnit injects it) but never forwarded to InitializeAsync. If InitializeAsync accepts a cancellation token, this should be passed through, otherwise the test timeout/cancellation from the test runner won't propagate.

Overly broad exception catch in WaitForResourceHealthyAsync_OnParameterResource_Hangs

catch (InvalidOperationException)
{
    timedOut = true;
}

Catching InvalidOperationException here could silently swallow unrelated failures (e.g., a programming error in the test setup) and incorrectly mark the test as passing. If this specific exception is expected from WaitForResourceHealthyAsync, add a comment explaining the scenario. Otherwise, remove it to keep the failure signal clean.

Summary

The core fix is correct and the regression tests are a great addition. The main open question is whether the opt-in IComputeResource filter is safe long-term, and the solution file + test organization items above. The hang reproduction test in particular is a nice piece of documentation-as-code.

Follow-up Code Review

Reviewing against the previous round of comments to check what's been addressed.

✅ Addressed from previous review

• TUnit.slnx missing projects — Both new projects are now added to both TUnit.slnx and TUnit.CI.slnx.
• HealthyFixture as top-level class — Now correctly a private sealed nested class inside WaitForHealthyReproductionTests.

Still open: ct not forwarded in AspireFixture_AllHealthy_Succeeds_AfterFix

// WaitForHealthyReproductionTests.cs
public async Task AspireFixture_AllHealthy_Succeeds_AfterFix(CancellationToken ct)
{
    var fixture = new HealthyFixture();
    try
    {
        await fixture.InitializeAsync();  // ct not passed
    }

The injected CancellationToken is still never forwarded. If InitializeAsync accepts one, pass ct so the test runner's timeout/cancellation propagates correctly.

Still open: broad InvalidOperationException catch

catch (InvalidOperationException)
{
    timedOut = true;
}

A programming error in test setup (e.g., misconfigured DI, wrong resource name) would also throw InvalidOperationException, and would incorrectly be counted as a pass. If this exception is genuinely expected from Aspire internals in this scenario, add a comment explaining which case triggers it. Otherwise, remove the catch to keep the signal clean.

Still open: opt-in IComputeResource filter semantics

The architectural concern from the previous review still stands — the flip from opt-out (IResourceWithoutLifetime) to opt-in (IComputeResource) means any future or custom lifecycle-bound resource that doesn't implement IComputeResource will be silently skipped. Whether that's acceptable is a design call, but it should at minimum be documented with a comment in GetWaitableResourceNames.

New: contradictory pipeline attributes in RunAspireTestsModule

[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
public class RunAspireTestsModule : Module<CommandResult>

RunOnLinuxOnly and RunOnWindowsOnly are both applied. If these attributes are ANDed (which is typical for attribute-based constraints), this module would never execute on any platform. Was this meant to be RunOnLinuxOnly alone (Aspire integration tests typically require Docker which is available on Linux CI runners)? Worth verifying the intended behavior of these combined attributes in ModularPipelines.

Overall

The core fix is correct and the test structure has improved (nested HealthyFixture, both solution files updated). The ct forwarding and broad exception catch are minor correctness issues; the pipeline attributes look like a potential bug worth double-checking.

Follow-up Code Review (Round 3)

Reviewing against the two previous rounds of comments to check what's been addressed.

✅ Addressed since last review

• ** catch** — now has a comment documenting that Aspire throws this for resources that can never become healthy. The rationale is clear.
• ** opt-in filter semantics** — comment added to GetWaitableResourceNames explaining the opt-in approach and why non-compute resources are skipped. Addresses the documentation concern.
• Test deduplication — VerifyIComputeResource.cs removed and merged into WaitForHealthyReproductionTests, reducing maintenance surface.

Still open: ct not forwarded in AspireFixture_AllHealthy_Succeeds_AfterFix

// WaitForHealthyReproductionTests.cs
public async Task AspireFixture_AllHealthy_Succeeds_AfterFix(CancellationToken ct)
{
    var fixture = new HealthyFixture();
    try
    {
        await fixture.InitializeAsync();  // ct is still not passed
    }

The injected CancellationToken from TUnit is accepted but never forwarded. If InitializeAsync doesn't have a CancellationToken overload, at minimum rename the parameter to _ to make the intentional discard explicit and suppress any linter warnings.

Still open: contradictory pipeline attributes in RunAspireTestsModule

[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
public class RunAspireTestsModule : Module<CommandResult>

Both RunOnLinuxOnly and RunOnWindowsOnly are applied simultaneously. If these attributes are ANDed as guards (typical for constraint attributes), this module would never run on any platform. The commit message says "Runs on Linux and Windows only" — if the intent is to run on both Linux and Windows, the correct approach is no platform restriction (or a RunOnLinuxOrWindows attribute if one exists). If it should only run on Linux (where Docker is available for Aspire), remove RunOnWindowsOnly.

Overall

The core fix is solid, the comment-level concerns from the previous review have been addressed, and the test structure is clean. The two remaining items are minor — the ct forwarding is a small correctness issue, and the contradictory pipeline attributes are worth a closer look to confirm the module actually runs in CI as intended.

Follow-up Code Review (Round 3)

Reviewing against the two previous rounds of comments to check what's been addressed.

Addressed since last review

• InvalidOperationException catch — now has a comment documenting that Aspire throws this for resources that can never become healthy. The rationale is clear.
• IComputeResource opt-in filter semantics — comment added to GetWaitableResourceNames explaining the opt-in approach and why non-compute resources are skipped. Addresses the documentation concern.
• Test deduplication — VerifyIComputeResource.cs removed and merged into WaitForHealthyReproductionTests, reducing maintenance surface.

Still open: ct not forwarded in AspireFixture_AllHealthy_Succeeds_AfterFix

// WaitForHealthyReproductionTests.cs
public async Task AspireFixture_AllHealthy_Succeeds_AfterFix(CancellationToken ct)
{
    var fixture = new HealthyFixture();
    try
    {
        await fixture.InitializeAsync();  // ct is still not passed
    }

The injected CancellationToken from TUnit is accepted but never forwarded. If InitializeAsync doesn't have a CancellationToken overload, at minimum rename the parameter to _ to make the intentional discard explicit and suppress linter warnings.

Still open: contradictory pipeline attributes in RunAspireTestsModule

[NotInParallel("NetworkTests"), RunOnLinuxOnly, RunOnWindowsOnly]
public class RunAspireTestsModule : Module<CommandResult>

Both RunOnLinuxOnly and RunOnWindowsOnly are applied simultaneously. If these attributes are ANDed as guards (the typical behavior for constraint attributes), this module would never run on any platform. The commit message says "Runs on Linux and Windows only" — if the intent is to run on both Linux and Windows, neither attribute should be applied (or a combined attribute used if one exists). If the intent is Linux-only (where Docker is available for Aspire), remove RunOnWindowsOnly. This is worth confirming so the new tests actually execute in CI.

Overall

The core fix is solid and the comment-level concerns from the previous review have been addressed cleanly. Two items remain: the ct forwarding (minor correctness issue) and the contradictory pipeline attributes (potential CI gap worth verifying).